Skip to content

feat: Add file loading code for flag overrides - #449

Merged
kinyoklion merged 18 commits into
feat/overridesfrom
rlamb/overrides-ruby-filedata
Nov 4, 2026
Merged

kinyoklion merged 18 commits into
feat/overridesfrom
rlamb/overrides-ruby-filedata

Conversation

@kinyoklion

@kinyoklion kinyoklion commented Sep 25, 2026 •

Copy link
Copy Markdown
Member

Summary

This is the first of a series that adds flag overrides, as defined by the OVERRIDE specification, to the Ruby server SDK. The existing FDv1 and FDv2 file data sources keep their current behavior: this PR does not change lib/ldclient-rb/impl/integrations/file_data_source.rb, file_data_source_v2.rb, or their specs, and the override feature is additive throughout the series.

The file-based override source must tolerate a missing file, a file that is being rewritten in place, and a burst of change notifications, and it must keep the last good data when a reload fails. This change adds LaunchDarkly::Impl::FileData, the file reading, parsing, merging, and reloading code that source needs:

  • Document parsing. The format comes from the file name: a name that ends in .yaml or .yml, in any case (a file named .yaml included), is YAML, and any other file, including one with no extension, is JSON. The parser checks that the document and its flags, flagValues, and segments members are objects; a JSON null, array or scalar is an error.
    • Whether an empty file is valid depends on the format. An empty or white-space JSON file fails to parse (the file has no content, which is not a JSON document). Empty or white-space YAML (a tab included), or YAML that is only a comment, --- or a null, is a document with no entries.
    • Files are read as bytes and taken as UTF-8, without a UTF-8 byte order mark, whatever the process's default encoding. Content that is not valid UTF-8 fails to parse in either format; a file with a UTF-16 or UTF-32 byte order mark is reported as such (encoded in UTF-16 or UTF-32; data files must be UTF-8).
    • JSON is parsed with JSON.parse, so JSON numbers are numbers (1e3 is 1000.0), and content after the document, a # comment or a trailing comma is an error. Comments and duplicate keys inside one object follow the application's json gem: 3.x rejects both.
  • An ordered merge of several documents with duplicate keys handling of fail or ignore, expansion of each flagValues entry into a flag that is on and serves the value by fallthrough (so an evaluation reports the FALLTHROUGH reason kind, as the specification describes), deserialization into the data model classes, and per-document entry counts. The key under which an entry appears is its key for every purpose: the item is stored under it, duplicates are detected by it, and the definition's own key is set to it, so evaluations report the flag by that key.
  • A reloader that serializes reloads, debounces change signals, keeps the last good result when a reload fails, retries after a bounded delay, reports each distinct failure once, and can skip a reload whose paths, presence and content are identical to the last applied ones. A configured file that does not exist can be made to contribute no data. An exception raised by the consumer's apply is treated as a failed reload: nothing from that load is remembered, the failure is reported, and the reload is retried. An error that a load does not expect is logged and retried, and the worker thread survives it.
  • In-place rewrites. Applications and editors truncate a file and then write the new content, and a load in between finds the file empty. A load that finds a file with no content (empty, white space, or YAML read as nothing) while the data in effect has content from that file holds the file back: it keeps the current data, logs at debug level, and arms the retry, without reporting a failure.
    • A file is held back until it has stayed so, unwritten, for one retry delay (1 second), timed from the clock read after that file's own read. A change of its state (modification time and size, read through any symbolic link with the poller's helper) is a new rewrite and starts the delay over, as does a read that finds it with content, even content that fails to parse, or missing.
    • Each file is held for at most five retry delays from its first hold; its bound starts over only at a load that holds nothing back, whether that load applies, is skipped, or fails.
    • A load reads and classifies every file, even after one fails to read or parse, so files emptied together are held and applied together; it then fails with the first failure, holds files back, or merges, in that order.
    • The retry of a hold is armed for the rest of the first due file's delay or bound, plus a tenth of the retry delay.
    • A file that contributed no content to the data in effect (a placeholder that has always been empty, a file that was missing, a file whose emptying has been applied) is parsed at once, and so is every file when retries are off.
    • After the delay, an empty JSON file fails to parse and keeps the data; an empty YAML file has no entries.
  • A stat-based poller that compares existence, modification time, and size, through symbolic links. A file that appears or disappears is a change.
  • A watcher that uses rb-inotify on Linux (a dependency of the optional listen gem) to watch the directory of each file without descending into subdirectories, and listen elsewhere. It picks up a file that does not exist yet when it appears, and it watches each directory on its own: a directory that does not exist is retried once per second while the others stay watched, and one change is signaled when a retry sets up a watch. Non-recursive watching matters because listen scans the whole tree under a watched directory and fails when a subdirectory cannot be read, as happens for a file in the system temporary directory. On Linux, a watched directory that is deleted is watched again once it exists, and one change is signaled so the reload picks up what was written meanwhile. A configured file that is a symbolic link is followed: the directory of the link's target is watched for the target's name, and the links are resolved again after every reported change, after a retry sets up watches, and when a directory that holds a target is deleted, as a Kubernetes ConfigMap update does. A link that cannot be resolved, for example because its target's directory does not exist yet or was deleted, is resolved again once per second until its target is watched, and one change is then signaled. A relative link is expanded against the real path of the directory that holds it, so a .. in it is right when a directory on the way is itself a link.

A new spec, spec/integrations/file_data_source_compatibility_spec.rb, pins the behaviors of the existing file data sources that their other specs did not assert (per-file version numbering, keying by the entry's own key, the on-plus-fallthrough expansion of flagValues, no retry of a failed load, log messages, modification-time-only polling, no reaction to a deleted file when polling, the FDv1 poller's reload cadence after a change, and use of listen when it is present), so that they stay as they are while this code exists beside them.

Not changed: YAML that is cut off mid-write can still parse, with fewer entries or a value cut short, so the override source docs recommend replacing a YAML file in one operation. A writer that pauses longer than the retry delay gets the empty file applied (YAML) or reported (JSON).

Testing

  • Unit specs for the document parser (format by name, empty JSON and YAML, UTF-8 and byte order marks, JSON numbers and trailing content), merge, poller (including symbolic links), reloader (debounce, retry, last-good retention, sequential reloads, stop, and the hold of an emptied file), and watcher (including a directory with an unreadable subdirectory, the inotify thread lifecycle, symbolic links to files in the same and in another directory, a link that is replaced, a ConfigMap-style update, a relative link in a directory reached through another link, and a target directory that appears late or is recreated), the new compatibility pins, and the unchanged existing file data source specs.
  • The reloader specs rewrite their files in place with real, deliberately slow writes (InPlaceWrites.rewrite in spec/in_place_writes.rb), and the hold's delays, bounds and retries are tested on a controlled clock with synchronous loads, with a few real-clock tests for the retry firing.
  • The in-place rewrite handling was reviewed in eight rounds as fix: Keep overrides through an in-place rewrite of their files #479, with multi-agent and Bugbot reviews, and is shared with the other SDKs' ports. On that code: a mutation check reverted each part of the fix on its own (67 mutants); 60 fail at least one test, and the other 7 are equivalent (the retries-off guard, the clearing of held-file records on apply or skip, starting over every file instead of the files read, and the path or the presence alone in the digest). The changed spec files ran 10 times in a row on Ruby 3.2 and on JRuby 9.4 with no failures.
  • Full suite and RuboCop are clean, apart from can be automatically loaded by Bundler.require, which needs write access to the system gem directory on the test machine. Each new spec was checked against a deliberate defect in the code it covers, and each compatibility pin against a deliberate change to the existing source it pins.

SDK-3249

Opened from termag

@kinyoklion
kinyoklion force-pushed the rlamb/overrides-ruby-filedata branch from eb26dd7 to 72d5776 Compare September 28, 2026 20:36
@kinyoklion kinyoklion changed the title feat: Make file data loading reliable and shared feat: Add file loading code for flag overrides Sep 28, 2026
@kinyoklion

Copy link
Copy Markdown
Member Author

bugbot review

@cursor cursor Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale Bugbot comment from a previous run.

value.map { |v| symbolize_keys(v) }
else
value
end

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

YAML on key silently disables flags

Medium Severity

YAML.safe_load treats unquoted on as a boolean, and symbolize_keys then stores that field as :true. Full flag definitions never see :on, so FeatureFlag treats them as off. Natural YAML like on: true therefore loads as a disabled flag instead of failing or preserving the on field.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 2e31db3. Configure here.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This reply was written by an AI agent (Claude Code) working on behalf of @kinyoklion.

Leaving this as is. Under YAML 1.1 an unquoted on key parses as the boolean true. Ruby's existing file data sources read YAML the same way (YAML.safe_load followed by symbolize_all_keys in file_data_source.rb and file_data_source_v2.rb on main), as do the Go and Python ports. The override docs (launchdarkly/sdk-meta#714, launchdarkly/ld-docs-private#8869) quote "on" in the YAML example and tell readers to do the same.

@kinyoklion

Copy link
Copy Markdown
Member Author

bugbot review

@cursor cursor Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale Bugbot comment from a previous run.

@kinyoklion

Copy link
Copy Markdown
Member Author

Follow-up to the poller-test change in the other ports: the reloader spec retries a failed reload after the retry delay and recovers when the file is fixed had the same flaw. It fixed the file with an ordinary File.write while the 0.1 s retry was pending and expected the retry to apply the fixed content. If the retry reads the file between the truncation and the write it sees an empty file, which is a valid document, so the retry applies an empty result and stops; the fixed content is never loaded and the spec fails with got: {}.

Reproduced locally with a deliberately slow write (truncate, sleep 0.3 s, write): the old expectation failed 3/3 and the new one passed 3/3. The spec now accepts the recovery the retry made and loads the fixed content on demand when the retry did not see it (5babf80). reloader_spec 5/5 runs green, spec/impl/file_data 76 examples green, and the file data plus override specs at the top of the stack (3758acf) 165 examples green. The stack above is rebased onto this commit.

Written by Claude Code on behalf of @kinyoklion.

def self.symbolize_keys(value)
case value
when Hash
value.to_h { |k, v| [k.to_s.to_sym, symbolize_keys(v)] }

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An operator who writes a YAML flag with an unquoted on: true gets a flag that is off, and nothing in the logs says why. YAML 1.1 reads the key on as the boolean true, and k.to_s.to_sym turns it into :true, so the flag has no on field and defaults to off. The same thing happens to flagValues keys named yes, no, on, or off.

Both existing file sources call k.to_sym here, which raises on true, so today the same file fails to load and the last good data stays in place. Can this raise too, with a message that tells the user to quote the key? That keeps the existing outcome and makes the fix obvious to whoever is editing the file during an incident.

Comment thread lib/ldclient-rb/impl/file_data/merge.rb Outdated
raise MergeError, "#{category} \"#{key}\" is not an object" unless data.is_a?(Hash)

data = data.dup
data[:key] = key.to_s if data[:key].nil?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This stores each entry under the key it appears under, and keeps a different key member if the entry has one. The existing sources store under the entry's own key, and the compatibility spec in this PR pins that ("stores an item under its own key member rather than the key it appears under").

Two things change for a user as a result. A file with flags: {a: {key: "b", ...}} overrides a, but evaluations report the flag as b in events. And two entries that share the same key under different map keys no longer fail as duplicates, which they do today. Can this key by data[:key] as the existing sources do, or reject an entry whose key doesn't match its map key?

@kinyoklion kinyoklion Nov 4, 2026 •

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This reply was written by an AI agent (Claude Code) working on behalf of @kinyoklion.

Changed in 2aa159b. The key an entry appears under is now its key for every purpose: the item is stored under it, duplicates are detected by it, and the definition's own key is set to it. So flags: {a: {key: "b", ...}} overrides a, and evaluations and events report it as a. Two entries under different keys are two flags, each reported by its own key, so no two items share a key. The existing file data sources keep keying by the entry's own key, as the compatibility spec pins. The Python port gets the same change in launchdarkly/python-server-sdk#525.

# is stopped.
#
private def watch_directory(directory)
real_directory = File.realpath(directory)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When a configured path is a symlink to a file in another directory, edits to that file are never picked up in watch mode. This resolves the real path of the directory, and events are then matched against the configured file name in that directory, so changes to the symlink's target never match.

Both existing sources resolve each file's real path and watch that file's directory (file_data_source.rb:161, file_data_source_v2.rb:370), so a user who points the existing source at a symlinked file sees reloads today. Can this watch the real path of each configured file to keep that working?

@kinyoklion kinyoklion Nov 4, 2026 •

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This reply was written by an AI agent (Claude Code) working on behalf of @kinyoklion.

Changed in c482b92. The watcher still watches the directory of each configured file, so a replaced or newly created file is seen, and for a configured file that is a symbolic link it now also watches the directory of the link's target for the target's name. The links are resolved again after every reported change, after a retry sets up watches, and when a directory that holds a target is deleted, so a link that is pointed elsewhere is followed, including the ..data swap of a Kubernetes ConfigMap update. A directory still has one watch whatever the reasons, because a second rb-inotify watch on it would replace the first one's callback. New watcher specs cover a link to a file in another directory, a link in the same directory, a link that is replaced, and a ConfigMap-style update. Polling was not affected, since File.stat follows the link. The Python port gets the same change in launchdarkly/python-server-sdk#525.

# a new delay. The retry that a hold arms comes when the first held file's
# delay or bound is over, plus a tenth of the retry delay. A file that contributed no content
# to the data currently applied, such as a placeholder that has been empty from the start, a
# file that was missing, or a file whose emptying has been applied, is parsed at once. So is every file when there are no

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Word wrap?

@kinyoklion kinyoklion Nov 4, 2026 •

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This reply was written by an AI agent (Claude Code) working on behalf of @kinyoklion.

Rewrapped in 01b1e65, with the paragraph's text unchanged. The Python port's reloader docstring had the same broken wrap and is rewrapped in launchdarkly/python-server-sdk#525.

Adds `LaunchDarkly::Impl::FileData`, the file reading, parsing, merging,
and reloading code that the file-based override source defined by the
OVERRIDE specification needs. That source must tolerate a missing file,
a file that is being written, and a burst of change notifications, and
it must keep the last good data when a reload fails. The module provides:

- Document parsing for JSON and YAML with validation that the document
  and its flags, flagValues, and segments members are objects.
- An ordered merge of several documents with duplicate keys handling of
  fail or ignore, flagValues expansion into off flags that serve the
  value, model deserialization, and per-document entry counts.
- A reloader that serializes reloads, debounces change signals, retains
  the last good result when a reload fails, retries after a bounded
  delay, reports each distinct failure once, and can skip byte-identical
  reloads. A configured file that does not exist can be treated as a
  file with no content.
- A stat-based poller that compares existence, modification time, and
  size, so a file that appears or disappears is a change.
- A watcher that uses rb-inotify on Linux to watch the directory of each
  file without descending into subdirectories, and the listen gem
  elsewhere, so a file that does not exist yet is picked up when it
  appears. It retries when a directory does not exist.

The existing FDv1 and FDv2 file data sources are not changed. They keep
their own implementation and behavior. A new spec pins the behaviors of
those sources that their other specs did not assert, so that they stay
as they are while this code exists beside them.
The result is not remembered as applied, on_error receives the exception, the reload is retried, and the worker thread continues.
The inotify watcher tears its watches down when a watched directory is deleted or moved, logs it, retries setup once per second, and signals one change when the watches are in place again.
Compare absolute paths through File.absolute_path, close temp files before deleting them, and backdate modification times instead of sleeping before a write that the pollers must see.
…pop timeouts

Queue#pop accepts a timeout only from Ruby 3.2 on. The sync examples also wait for the poller thread or the listen call before they act, because the source starts change detection after it yields the initial data.
The watcher set up its watches all or nothing. If any directory of the configured paths did not exist, it logged the problem, watched nothing, and retried once per second until every directory existed. A configured file in a directory that never exists is a valid steady state for the override source, which treats a missing file as contributing nothing, so one permanently missing directory meant that changes to files in the directories that do exist were never detected.

Each directory is now watched on its own. At start, the directories that exist are watched at once and the others are remembered as missing and retried once per second. A retry adds the watches for the directories that exist by then, and the callback runs once when it added at least one, whether or not other directories are still missing. The directories that are still missing, and any watch that could not be set up, are logged together, so that an unchanged situation repeats at debug level as before. When a watched directory is lost, only its watch is removed and it goes back to the missing set; the other directories keep their watches. With rb-inotify, one notifier and one thread take the watches as they are added, and a lost directory's watch is closed. With the listen gem, there is one listener per directory, and stop ends each of them.

The spec that lost a directory as soon as its watches were set up waited for the inotify thread to end as its sign that the loss had been handled. The thread outlives one directory now, so it waits for the warning in the log instead. New specs cover a directory missing at start beside one that exists, a lost directory watched again while another is still missing, and stop while a directory is missing.
The spec fixed a failing file while the automatic retry was pending and expected the retry to
apply the fixed content. The write is ordinary, not atomic, so the retry can read the file while
it is still empty; an empty document is valid and is applied as the recovery, and the fixed
content is never loaded. The spec now accepts the recovery the retry made and loads the fixed
content on demand when the retry did not see it.
…by file name

Applications and editors rewrite files in place: they truncate the file,
then write the new content. A load in between found the file empty and
applied it as a document with no entries.

- The format comes from the file name: a name that ends in .yaml or .yml,
  in any case, is YAML, and any other file is JSON. An empty or white
  space JSON file is not a JSON document and fails to parse; empty YAML,
  or YAML that is only a comment or "---", has no entries. Files are read
  as bytes and taken as UTF-8 without a byte order mark, and content that
  is not valid UTF-8 fails to parse.
- A load that finds a file with no content while the data in effect has
  content from it holds the file back: it keeps the current data and arms
  the retry without reporting a failure. The file is held until it has
  stayed so, unwritten (its modification time and size, through any
  symbolic link), for one retry delay, timed from its own read; a new
  rewrite starts the delay over. Each file is held for at most five retry
  delays from its first hold, and its bound starts over at a load that
  holds nothing back. The retry of a hold is armed for the rest of the
  delay plus a tenth of it.
- A load reads every file even after one fails, and fails with the first
  failure. Content, even content that fails to parse, or a missing file
  ends a file's empty state. The skip-unchanged digest covers each path
  and whether its file is present.
- An error that a load does not expect is logged and retried, and the
  worker survives it.

The tests rewrite files in place with real, deliberately slow writes, and
time the holds on a controlled clock.
…ment

An entry's own key member was kept when it differed from the key the entry appears under. The item was stored under one key and reported by evaluations under the other, and two entries with the same key member under different keys were two items with one key. The key the entry appears under is now the item's key for every purpose.
The watcher matched events in the directory of each configured file against the file's name, so an edit to the target of a configured symbolic link in another directory was never reported. The watcher now also watches the directory of each link's target for the target's name, and resolves the links again after every reported change, after a retry sets up watches, and when a directory that holds a target is deleted, as a ConfigMap update does.
…tched

A link whose target's directory did not exist when the links were resolved, at start or after the directory was deleted, was not resolved again until another change was reported, so edits to its target after the directory appeared were missed. Such a link, or a target directory that cannot be watched, now keeps the once-per-second retry running, which resolves the links again, and the callback runs once when a retry watches a target.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

There are 2 total unresolved issues (including 1 from previous review).

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 809c4e7. Configure here.

…holds it

A relative link was expanded against the path of its directory as written, so a ".." in it went to the wrong place when a directory in that path was itself a link, as when a current link points to a release directory. The link is now expanded against the real path of the directory that holds it.
@kinyoklion
kinyoklion force-pushed the rlamb/overrides-ruby-filedata branch from 809c4e7 to a43c443 Compare November 4, 2026 19:48
@kinyoklion
kinyoklion merged commit bfcbb81 into feat/overrides Nov 4, 2026
11 of 12 checks passed
@kinyoklion
kinyoklion deleted the rlamb/overrides-ruby-filedata branch November 4, 2026 20:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants